Skip to content
This repository was archived by the owner on Jan 24, 2018. It is now read-only.

[WIP] Create metric and billing interface outline #173

Merged
merged 5 commits into from
Mar 14, 2017

Conversation

lasley
Copy link
Contributor

@lasley lasley commented Dec 1, 2016

This is a prelim outline of what I have been describing in #157. Nothing functional at the moment other than data schemas and a few theoretical methods.

There's also a shell of a metric interface definition, but that was just so I had something to connect the billing models to. I'm still coming up with a real plan there.

invoice_policy = fields.Selection(
selection_add=[
('threshold', 'Invoice and Enforce a Threshold'),
('usage', 'Invoice Based on Usage'),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be removed in favor of using pre-existing delivery policy, which represents the same thing for the most part

@lasley lasley force-pushed the feature/master/usage-and-billing branch 2 times, most recently from e88d57f to b0bff52 Compare December 1, 2016 04:03
@@ -0,0 +1,5 @@
# -*- coding: utf-8 -*-
# Copyright 2016 LasLabs Inc.
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change all to LGPL

@YannickB
Copy link
Owner

YannickB commented Dec 1, 2016

Great @lasley , I had a first look at it but I'll have a second one tomorrow. Thanks!

Copy link
Owner

@YannickB YannickB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comment, but other than that LGTM

required=True,
ondelete='restrict',
)
service_id = fields.Many2one(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget that we shall also be able to invoice a base, not only a service.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good call, I knew it seemed like I was missing a layer of abstraction!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so I'm clear, what metrics are going to be monitored on base? That's just going to be flat rate for the most part right?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum the main one I see we can use for billing is the number of users

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm but wouldn't the number of users be dictated in the Service itself? The base represents a collection of services doesn't it?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope it's the other way around.

A service is a set of containers, aka one Odoo instance.
A base represent both a database and an url. You can have several database in one service, like the Odoo SA offer.

And you want to count the number of user for each database of course.

from odoo import api, fields, models


class ClouderMetricValue(models.Model):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand the use of this model, because we said the value would be recovered realtime from ELK. If we need to store it here, we shall probably have a datetime field to know when the value was recovered.

That said, I understand this is a POC and it will probably change when we get close to the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah everything is going to be stored in ELK, but we need some way to store the aggregate billing metrics so they don't change out from under us after they have been used for billing.

Yup you're totally right about the need for datetimes indicating the aggregation interval. I added that in my RFC at https://github.com/clouder-community/clouder/issues/174

This module is likely going to be removed from this PR, it was just here so I could connect accounting to something while figuring that all out.

@lasley lasley force-pushed the feature/master/usage-and-billing branch 2 times, most recently from bca9547 to 3051df9 Compare December 20, 2016 19:49

@api.multi
def _get_quantity_usage(self, account_line, invoice):
""" It provides a quantity based on unbilled and used metrics """

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @lasley,

My assumption is that we need to get a RecordSet of clouder.metric.interface to return here, right? I'm just failing to find a good relation between the two. Perhaps it just wasn't implemented yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ummm no, it would be an actual numeric. All of these _get_quantity_* methods would be used to determine the product_qty of the invoice line item.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at _get_quantity_flat

@tedsalmon tedsalmon force-pushed the feature/master/usage-and-billing branch from 3f9e1ff to 41717e5 Compare March 9, 2017 02:06
@codecov-io
Copy link

codecov-io commented Mar 10, 2017

Codecov Report

Merging #173 into master will increase coverage by 1.37%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #173      +/-   ##
==========================================
+ Coverage   31.53%   32.91%   +1.37%     
==========================================
  Files          73       79       +6     
  Lines        5755     5873     +118     
==========================================
+ Hits         1815     1933     +118     
  Misses       3940     3940
Impacted Files Coverage Δ
sale_clouder/models/clouder_contract_line.py 100% <100%> (ø)
clouder_metric/models/clouder_metric_interface.py 100% <100%> (ø)
sale_clouder/models/clouder_contract.py 100% <100%> (ø)
sale_clouder/models/product_template.py 100% <100%> (ø)
clouder_metric/models/clouder_metric_type.py 100% <100%> (ø)
clouder_metric/models/clouder_metric_value.py 100% <100%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b275e75...400f151. Read the comment docs.

@tedsalmon tedsalmon force-pushed the feature/master/usage-and-billing branch 2 times, most recently from bbfdc58 to 92cafec Compare March 11, 2017 00:05
@tedsalmon
Copy link

Hey guys,

This is should be ready for review. Let me know if I missed anything.

Thanks!
-Ted

@tedsalmon tedsalmon force-pushed the feature/master/usage-and-billing branch from 92cafec to d57d815 Compare March 13, 2017 17:32
@@ -0,0 +1,23 @@
# -*- coding: utf-8 -*-
# Copyright 2016 LasLabs Inc.
# License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has to be AGPL due to contract_variable_quantity depends - assuming we're still using logic from it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh... This may be a problem, the metric module will probably be deployed in all infrastructures. If it becomes AGPL, we can practically assume that all Clouder will become AGPL, this is not the same case than clouder_asynchronous.

I'm afraid I have to be against such move just because we use this dependency. Either we can convince people that contract_variable_quantity is considered a component and shall be LGPL, or we'll have to do without it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being said, I think I'll not block the PR just for that, we can find a solution later and merge it AGPL for the time being.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YannickB - I think we'll be fine here. Reason being is that the metrics and sales modules are far from the core, nothing will need to depend on them, and there will likely not be third party proprietary changes.

Anything to do with contract is licensed AGPL, primarily due to it being ripped out from v8 - before the LGPL license switch in v9. I don't really want to rewrite the contract module at this time, but that would be the requirement to move from this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very well, I'm fine anyway we'll figure out what happen later.

This object receives all attributes from ``clouder.metric.type``.
"""

_name = 'clouder.metric.interface'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_description key on all new models

class ClouderMetricType(models.Model):
""" It provides context for usage metric types """

_name = 'clouder.metric.type'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_description key on all new models

return _("# Python code. \n"
"Use `value = my_value` to specify the final calculated "
" metric value. This is required. \n"
"Optionally use ``uom == product_uom_record`` to change the "
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/==/=

"units that the metric is being measured in. \n"
"# You can use the following variables: \n"
"# - self: browse_record of the current ID Category "
"browse_record. \n"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double browse_record

line being referenced.
invoice (account.invoice): Invoice that is being created.
Returns:
(float) Quantity with no calculations performed
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

float:

line being referenced.
invoice (account.invoice): Invoice that is being created.
Returns:
(float) Quantity to use on invoice line in the UOM defined on the
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

float:

return False
start = fields.Datetime.from_string(rec.date_start)
end = fields.Datetime.from_string(rec.date_end)
return start >= start_date and end <= inv_date
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add blank line

end = fields.Datetime.from_string(rec.date_end)
return start >= start_date and end <= inv_date
usage_values = vals.filtered(filter)
for val in usage_values:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usage = sum(usage_values.mapped('value'))

class ClouderContractLine(models.Model):
""" It provides the link between billing and Clouder Services. """

_name = 'clouder.contract.line'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_description

lasley and others added 4 commits March 13, 2017 14:04
* Fix docstrings
* Implement unimplemented methods
* Fix bugs found
* Add test coverage
* Add README

[IMP] clouder_metric: Finish module
* Fix/Add docstrings
* Add test coverage
* Fix logic
* Add README
* Update license to AGPL
* Add model `_description`
* Clean up lint issues
@tedsalmon tedsalmon force-pushed the feature/master/usage-and-billing branch from 489375c to 8c7645c Compare March 13, 2017 21:05
* Update license to AGPL
* Add model `_description`
* Clean up lint issues
* Improve usage metric aggregation logic
@tedsalmon tedsalmon force-pushed the feature/master/usage-and-billing branch from 297f6c7 to 400f151 Compare March 13, 2017 21:45
@tedsalmon
Copy link

Thanks @lasley! I've attended to your comments in commit 8c7645c & 400f151

Again, I was unable to switch the fields.Selection field to fields.Reference because the latter is a reference to a specific record of a model; not the model itself.

I also boiled down some of the logic used to calculate the usage quantity in clouder.contract

Thanks!

@YannickB
Copy link
Owner

Hi guys,

Good job ! I'll make my review this afternoon, hope we can merge it quickly after that. Thanks again for your hard work.

Copy link
Owner

@YannickB YannickB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, except one minor comment, I'm ok for this PR.

Note I've yet to make a functional review. I think make optimistic merge and correct functional issues through next PRs is the way to go here, so I'm ready to merge when @lasley approve.

@@ -0,0 +1,25 @@
# -*- coding: utf-8 -*-
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make a symbolic link from openerp.py to manifest.py to ensure the module can be installed on Odoo 9 (see my last commit on master)

@@ -0,0 +1,23 @@
# -*- coding: utf-8 -*-
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make a symbolic link from openerp.py to manifest.py to ensure the module can be installed on Odoo 9 (see my last commit on master)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so sure about this. What about the changed namespace (from odoo vs from openerp)? I'm pretty sure I have also used some new API decorators that will not work on v9 (api.model_cr_context)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well at the very least the tricks worked on my dev instance, and all other modules now have the symbolic link. Anyway I'll not block the PR for this, so I'm just waiting for your approval.

@lasley
Copy link
Contributor Author

lasley commented Mar 14, 2017

LGTM thanks @tedsalmon. We'll likely need to make some changes when we actually implement with the Elastic connector, but this is a great start for a merge IMO.

@YannickB
Copy link
Owner

OK, LGTM for me too then. I'm merging, thanks @tedsalmon @lasley for the hard work !

@YannickB YannickB merged commit 59dd6ea into YannickB:master Mar 14, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants